new: DefaultACL options for server config #7111

dblythy · 2021-01-06T05:27:58Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

Closes: #7068

Approach

Adds a "defaultACL" option to Parse Server config.

This sets the ACL on Parse.Objects if they are unset. It does not override ACL.

Default ACL is a JSON representation of Parse.ACL, except you can set currentUser, which overrides to the user who saves the object.

TODOs before merging

Add test cases
Add entry to changelog
Check for the user class
Add changes to documentation (guides, repository pages, in-code descriptions)

Moumouls

Here I ask myself one thing:
If we have a defaultACL config by Class, why not use a new field into Parse.Schema. To allow further remote control (via Parse Dashboard) and keep one source of truth.

The current string/object system seems to not support the use case:

User wants to apply same defaultACL on all classes but wants also to apply a specific default ACL config on one class

Here I would suggest to support only the object version in parse options. Less doc variation, more consistency over parse ecosystem. The ACL defined into the defaultAcl parse server options, will be used as a fallback if no defaultACL is found on the Class Schema.

Then support a new field defaultACL on Parse.Schema (like the clp field)

Additional note: Having a new defaultACL field on Class could work nicely with the new defined schema system.
Then the developer will have in one place: className, field, indexes, CLP, defaultACL.

What do you think about this ?

PS: Here i know that it need more work, but i think it's one of the best ways to achieve this feature, also we can after update the JS SD. (currently i don't know if other SDKs are actively used to manage schemas)

dblythy · 2021-01-06T07:29:26Z

I like the suggestion @Moumouls, so defaultACL in server options should only be a string which fallbacks if Schema defaultACL isn't defined?

I also think this PR requires extensive review to make sure no ACL is being overriden, which could obviously be a security concern if defaultACL is public, and a Parse.Object has ACL set to private.

codecov · 2021-01-06T07:32:03Z

Codecov Report

Merging #7111 (fec2f54) into master (8ff0d08) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7111      +/-   ##
==========================================
+ Coverage   93.64%   93.69%   +0.05%     
==========================================
  Files         169      169              
  Lines       12508    12537      +29     
==========================================
+ Hits        11713    11747      +34     
+ Misses        795      790       -5

Impacted Files	Coverage Δ
src/Options/index.js	`100.00% <ø> (ø)`
src/Config.js	`91.90% <100.00%> (+0.44%)`	⬆️
src/Options/Definitions.js	`100.00% <100.00%> (ø)`
src/RestWrite.js	`94.03% <100.00%> (+0.35%)`	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`93.51% <0.00%> (+0.67%)`	⬆️
src/Adapters/Files/GridFSBucketAdapter.js	`80.32% <0.00%> (+0.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ff0d08...fec2f54. Read the comment docs.

Moumouls · 2021-01-06T08:03:51Z

so defaultACL in server options should only be a string which fallbacks if Schema defaultACL isn't defined?

Here i think we should keep a consistent usage of ACL and avoid a new string ACL feature. So let's just use a the current ACL spec (follow Rest spec): https://docs.parseplatform.org/rest/guide/#user-security

defaultACL: {
  "*": {
    read: true
    write: false
  }
  "role:Admin":{ read: true, write: true}
...
}

I understand that this structure do not support your currentUser ACL. But here i would suggest to work on this in another PR.

I think that the currentUser ACL is missing in ACL definitions, and this feature will be easy to understand for developers on APIs (ex: GraphQL) or the new defined schema.

Current user feature could be achieved by simply transforming on the fly on parse server side the currentUser to req.auth.id 🙂

currentUser: {
  read: true,
  write: false
}

Also in graphql this feature would be very interesting:

mutation createObject {
  createComment(input: { fields: { ACL: { currentUser: { read: true, write: false} } } }) {
    comment {
      id
      ACL {
        users {
          # Current user is now listed here
          userId
        }
      }
    }
  }
}

Moumouls · 2021-01-06T08:19:29Z

@dblythy

I also think this PR requires extensive review to make sure no ACL is being overriden

You should only trigger default ACL on object creation, never on object update :)

it's too hard to manage ACL on update and avoid conflicts, if a developer need more control on this part, he should use the beforeSave trigger to introduce his business logic.

Note on creation if the user provide ACL you can merge the ACL this way to ensure default and custom one.

ACL = {...this.data.ACL, ...defaultACL}

This way we let the user add additional ACL and ensure that default ACL correctly overwrite ACL provided by the user. (for example if malicious user try to disable the write of the role:Admin)

dblythy · 2021-01-06T10:05:21Z

@Moumouls how would you feel about evolving this feature in the future as part of the WIP for schema?

Schema evolution:

-Validator (#7013)
-defaultACL (related to this PR)

So in the future, you'll be able to set class specific data validation and class specific defaultACL via the dashboard.

Or would you prefer the defaultACL schema to be part of this PR?

Moumouls · 2021-01-06T10:31:42Z

Yes to keep the current PR scope small as possible, here we just need to support defaultACL for all classes via ParseServer Options

Then another PR could cover the defaultACL on Parse.Schema.

Moumouls

Add an explicit test in ACL.spec, check if currentUser works correctly via the create/update REST API

Moumouls · 2021-01-06T10:45:35Z

src/RestWrite.js

+  if (reqUser && aclOptions.currentUser) {
+    aclOptions[reqUser] = aclOptions.currentUser;
+  }


this code is not used at the right place.
You need to add an explicit test create/update ACL with the Rest API into spec/ParseACL.spec.js

Then the rest / graphql API could use this new field for better DX.

Here you only trigger the ACL transformation at creation time and if user do not provided ACL.

May be create a new restwrite fn handleACL to perform the operation after buildDefaultACL.

I have added a test case for REST API and it works fine with currentUser

Moumouls · 2021-01-06T10:46:19Z

src/RestWrite.js

+      write: true,
+    },
+  };
+  const aclOptions = Object.assign({}, this.config.defaultACL || defaultACL);


Suggested change

const aclOptions = Object.assign({}, this.config.defaultACL || defaultACL);

const aclOptions = this.config.defaultACL || defaultACL;

If I do that, tests fail and this.config.defaultACL gets mutated and then other tests fail.

Moumouls · 2021-01-06T10:49:08Z

src/RestWrite.js

+RestWrite.prototype.buildDefaultACL = function () {
+  if ((this.query && this.query.objectId) || this.data.ACL) {
+    return;
+  }


if user provide ACL on new object you need to combine the default and provided ones like this:

finalAcl = {...this.data.ACL, ...defaultACL}

Here we need to ensure that defaultACL at creation time is always merged and may be overwrite some unwanted ACL (user provide: role:Admin: { read: true, write: false}, but we want role:Admin: { read: true, write: true})

I think for this PR we should only affect objects with intrinsic no ACL, otherwise if a user wants to use the legacy option of defaultACL: {'*': { read: true, write: true }}, they have no option to make objects private.

E.g:

defaultACL: {'*': { read: true, write: true }}

Create object with new Parse.ACL(user);

The object ACL will 'merge' to be:

{ h8qM6TzGqA: { read: true, write: true }, '*': { read: true, write: true } }

dblythy · 2021-01-10T15:08:45Z

TODO: check how this functions for Parse.User.signUp()

mtrezza · 2021-09-03T00:49:13Z

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

We are planning to release the first beta version of Parse Server 5.0 in October 2021.

If a PR contains a breaking change and is not merged before the beta release of Parse Server 5.0, it cannot be merged until the end of 2022. Instead it has to follow the Deprecation Policy and phase-in breaking changes to be merged during the course of 2022.

One of the most voiced community feedbacks was the demand for predictability in breaking changes to make it easy to upgrade Parse Server. We have made a first step towards this by introducing the Deprecation Policy in February 2021 that assists to phase-in breaking changes, giving developers time to adapt. We will follow-up with the introduction of Release Automation and a branch model that will allow breaking changes only with a new major release, scheduled for the beginning of each calendar year.

We understand that some PRs are a long time in the making and we very much appreciate your contribution. We want to make it easy for PRs that contain a breaking change and were created before the introduction of the Deprecation Policy. These PRs can be merged with a breaking change without being phased-in before the beta release of Parse Server 5.0. We are making this exception because we appreciate that this is a time of transition that requires additional effort from contributors to adapt. We encourage everyone to prepare their PRs until the end of September and account for review time and possible adaptions.

If a PR contains a breaking change and should be merged before the beta release, please mention @parse-community/server-maintenance and we will coordinate with you to merge the PR.

Thanks for your contribution and support during this transition to Parse Server release automation!

dblythy · 2021-09-10T00:25:42Z

Closing as probably best handled in CLPs

dblythy added 6 commits December 15, 2020 00:06

new: defaultACL options for server config

65745dc

check default configuration

443b4c6

Update RestWrite.js

d88047e

Merge branch 'master' into defaultACL

e04a4fb

run defintions

2bdeca5

Update CHANGELOG.md

80829a0

Moumouls suggested changes Jan 6, 2021

View reviewed changes

fix failing tests

2d3a6b8

dblythy marked this pull request as draft January 6, 2021 08:49

dblythy added 4 commits January 6, 2021 20:26

change to currentUser

d9b6b2a

fix tests

e3a1426

validate config

487e95e

Update buildConfigDefinitions.js

b60f8a3

Update RestWrite.js

5259957

dblythy marked this pull request as ready for review January 6, 2021 10:29

Moumouls suggested changes Jan 6, 2021

View reviewed changes

Update ParseACL.spec.js

fec2f54

dblythy closed this Sep 10, 2021

dblythy deleted the defaultACL branch September 10, 2021 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new: DefaultACL options for server config #7111

new: DefaultACL options for server config #7111

dblythy commented Jan 6, 2021 •

edited

Loading

Moumouls left a comment

dblythy commented Jan 6, 2021 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading

Moumouls commented Jan 6, 2021

Moumouls commented Jan 6, 2021 •

edited

Loading

dblythy commented Jan 6, 2021 •

edited

Loading

Moumouls commented Jan 6, 2021 •

edited

Loading

Moumouls left a comment

Moumouls Jan 6, 2021

dblythy Jan 6, 2021

Moumouls Jan 6, 2021

dblythy Jan 6, 2021

Moumouls Jan 6, 2021

dblythy Jan 6, 2021

dblythy commented Jan 10, 2021

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

dblythy commented Sep 10, 2021

	const aclOptions = Object.assign({}, this.config.defaultACL \|\| defaultACL);
	const aclOptions = this.config.defaultACL \|\| defaultACL;

new: DefaultACL options for server config #7111

new: DefaultACL options for server config #7111

Conversation

dblythy commented Jan 6, 2021 • edited Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

Moumouls left a comment

Choose a reason for hiding this comment

dblythy commented Jan 6, 2021 • edited Loading

codecov bot commented Jan 6, 2021 • edited Loading

Codecov Report

Moumouls commented Jan 6, 2021

Moumouls commented Jan 6, 2021 • edited Loading

dblythy commented Jan 6, 2021 • edited Loading

Moumouls commented Jan 6, 2021 • edited Loading

Moumouls left a comment

Choose a reason for hiding this comment

Moumouls Jan 6, 2021

Choose a reason for hiding this comment

dblythy Jan 6, 2021

Choose a reason for hiding this comment

Moumouls Jan 6, 2021

Choose a reason for hiding this comment

dblythy Jan 6, 2021

Choose a reason for hiding this comment

Moumouls Jan 6, 2021

Choose a reason for hiding this comment

dblythy Jan 6, 2021

Choose a reason for hiding this comment

dblythy commented Jan 10, 2021

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

dblythy commented Sep 10, 2021

dblythy commented Jan 6, 2021 •

edited

Loading

dblythy commented Jan 6, 2021 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading

Moumouls commented Jan 6, 2021 •

edited

Loading

dblythy commented Jan 6, 2021 •

edited

Loading

Moumouls commented Jan 6, 2021 •

edited

Loading